http: call _writeRaw callback when destroyed#63447
Conversation
Fixes: nodejs#36673 Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: openai:gpt-5.5
|
Review requested:
|
nodejs-github-bot
added
http
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63447 +/- ##
==========================================
- Coverage 90.06% 90.04% -0.02%
==========================================
Files 714 714
Lines 225918 225928 +10
Branches 42734 42732 -2
==========================================
- Hits 203464 203438 -26
- Misses 14232 14275 +43
+ Partials 8222 8215 -7
🚀 New features to boost your workflow:
|
trivikr
added
author ready
github-actions
Bot
removed
the
request-ci
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Two small comments here, I could easily be persuaded to ignore these if that's complicated for some reason.
One larger issue though: with this PR in place, this code:
'use strict';
const http = require('http');
const { OutgoingMessage } = http;
const server = http.createServer((req, res) => {
res.on('finish', () => console.log('finish'));
res.on('error', (e) => console.log(`error:${e.code}`));
res.on('close', () => {
console.log('close');
server.close();
process.exit(0);
});
req.socket.destroy();
console.log('aborted with destroy()');
setImmediate(() => {
// Write after socket destroyed:
res.end('x');
console.log('Wrote after socket destroyed');
});
});
server.listen(0, '127.0.0.1', () => {
http.get(`http://127.0.0.1:${server.address().port}/`).on('error', () => {});
});Now prints:
aborted with destroy()
Wrote after socket destroyed
finish
close
I.e. with this change, we now emit 'finish' when ending aborted responses. Without this it skips finish and just closes.
I think that's because the onFinish callback used by end() ignores its error argument and just checks outmsg?.socket?._hadError, which isn't set here.
|
|
||
| if (this.destroyed) { | ||
| if (typeof callback === 'function') { | ||
| process.nextTick(callback, new ERR_STREAM_DESTROYED('write')); |
There was a problem hiding this comment.
In the equivalent cases elsewhere (like this) we do something like this.errored ?? new ERR_STREAM_DESTROYED('write') so that specific errors here are exposed if set, and ERR_STREAM_DESTROYED is just a fallback. Not strictly required but would nice imo.
| // The socket was destroyed. If we're still trying to write to it, | ||
| // then we haven't gotten the 'close' event yet. | ||
| if (typeof callback === 'function') { | ||
| process.nextTick(callback, new ERR_STREAM_DESTROYED('write')); |
There was a problem hiding this comment.
Ditto, for errors from the socket itself.
Both could alternatively do { cause: this.errored } instead I suppose to expose underlying errors as a cause instead, if that's preferable.
4 participants
OutgoingMessage._writeRaw()could return early without invoking the writecallback when the message, or its socket, was already destroyed.
This updates
_writeRaw()to call the callback withERR_STREAM_DESTROYEDin those cases, matching the existing destroyed
write()behavior.Fixes: #36673
Assisted-by: openai:gpt-5.5